Layout: Show vertical alignment toolbar with allowSwitching enabled #67022
Conversation
Remove layout.allowSwitching condition to display vertical alignment toolbar when flex layout is active.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Mamaduka
added
[Type] Bug
| if ( layoutBlockSupport?.allowSwitching ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I think this early return condition needs to be fixed. It should probably return null when both allowJustification and allowVerticalAlignment are false.
@ntsekouras, can you confirm? I think you worked on the initial implementation of this feature.
There was a problem hiding this comment.
It's been so long and even by looking at old PRs, I'm still not 100% sure why this check was added. If I remember correctly at that time we explicitly didn't want to show the justification controls in inspector controls too. Having said that, back then there were fewer layout props and there was no vertical alignment.
The allowJustification prop was added specifically for constraint layout but in docs says that can also be applied to flex and defaults to true. There are no checks for it now in flex layout, so I guess we need to take into account both in inspectorControls and toolBarControls.
So, I'm not sure whether the allowSwitching check is still needed, but the other checks (allowVerticalAlignment, allowJustification) should be added.
I'll cc @tellthemachines if she has more context.
There was a problem hiding this comment.
I don't have any further context but agree this check isn't the right one here.
One thing I'm noticing now is that setting allowJustification to false doesn't actually disable the justification controls for flex layout either in the toolbar or the sidebar. That should also be addressed, but not necessarily as part of this PR.
Given that this change by itself fixes #67016 and doesn't affect anything else, I'm ok with merging this as is and fixing the justification controls separately.
| if ( layoutBlockSupport?.allowSwitching ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I don't have any further context but agree this check isn't the right one here.
One thing I'm noticing now is that setting allowJustification to false doesn't actually disable the justification controls for flex layout either in the toolbar or the sidebar. That should also be addressed, but not necessarily as part of this PR.
Given that this change by itself fixes #67016 and doesn't affect anything else, I'm ok with merging this as is and fixing the justification controls separately.
tellthemachines
added
the
props-bot
github-actions
bot
removed
the
props-bot
|
I just opened #67059 to add some |
Fixes: #67016
What?
Enable vertical alignment toolbar to display when block layout has allowSwitching enabled and flex layout is active.
Why?
Currently, the vertical alignment toolbar is hidden when allowSwitching is set to true, even when flex layout is selected.
Testing Instructions
Screenshots or screencast
Before Fix:
After Fix: